Set default dashboard for users and fix a bug in preferences for default dashboard. #3781
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #3781 +/- ##
==========================================
- Coverage 45.72% 45.7% -0.02%
==========================================
Files 280 280
Lines 21134 21140 +6
Branches 3285 3286 +1
==========================================
Hits 9663 9663
- Misses 11471 11477 +6
Continue to review full report at Codecov.
|
core/controllers/profile.py
Outdated
| @@ -243,6 +245,12 @@ def get(self): | |||
| """Handles GET requests.""" | |||
| return_url = str(self.request.get('return_url', self.request.uri)) | |||
|
|
|||
| # Check if the return url is the creator dashboard. If it is, we should | |||
| # set the default dashboard of the user as the creator dashboard. | |||
| if feconf.CREATOR_DASHBOARD_URL in return_url: | |||
There was a problem hiding this comment.
I think you should do this in a later stage, only after the user has actually signed up. In general, avoid state-changing operations in GET requests.
| @@ -72,13 +72,13 @@ | |||
| <div class="col-lg-10 col-md-10 col-sm-10"> | |||
| <div class="checkbox" style="padding-top: 0;"> | |||
| <label> | |||
| <input type="radio" ng-model="defaultDashboard" value="DASHBOARD_TYPE_CREATOR" ng-change="saveDefaultDashboard(defaultDashboard)"> | |||
| <input type="radio" ng-model="defaultDashboard" value="<[DASHBOARD_TYPE_CREATOR]>" ng-change="saveDefaultDashboard(defaultDashboard)"> | |||
There was a problem hiding this comment.
Optional: would ng-value be better? I'm not sure.
Also, is this actually an error in production -- i.e. is a hotfix needed?
There was a problem hiding this comment.
Should we also have e2e tests to verify that the default dashboard is changed when the preferences are changed?
| urlService.getUrlParams().return_url); | ||
|
|
||
|
|
||
| // If the user has the return url as the creator dashboard, it is |
There was a problem hiding this comment.
Why do you not do this in the backend POST request, to avoid a second RPC and a potential race condition?
There was a problem hiding this comment.
Don't forget the back end tests, too.
There was a problem hiding this comment.
I am facing with finding the return URL. Unfortunately, in the POST request for the signuphandler I do not have the return url part. Should I pass it as a backend parameter? That would be a lot easier I guess. Right now it's very difficult to what I've implemented. What do you think about this method?
There was a problem hiding this comment.
I'd say, just pass a variable that's called default_dashboard or similar. The handler doesn't need the full URL.
| @@ -124,6 +124,23 @@ oppia.controller('Signup', [ | |||
| agreed_to_terms: agreedToTerms, | |||
| can_receive_email_updates: null | |||
| }; | |||
|
|
|||
| var defaultDashboard = ''; | |||
There was a problem hiding this comment.
Suggest defensively programming this so that it defaults to "learner" here. That way the value can never be invalid, even if further logic is added in the future.
core/controllers/profile.py
Outdated
| can_receive_email_updates = self.payload.get( | ||
| 'can_receive_email_updates') | ||
|
|
||
| has_ever_registered = user_services.has_ever_registered(self.user_id) | ||
| has_fully_registered = user_services.has_fully_registered(self.user_id) | ||
|
|
||
| # Set the default dashboard for new users. |
There was a problem hiding this comment.
No, this should only happen if the user has never registered in the past, and registration succeeds, right? So basically it should go around or after line 321:
if not has_ever_registered:
# do the default dashboard stuff
Note that this handler can get called again by existing users if we update the terms and ask them to agree to them. Or if they click the Back button.
There was a problem hiding this comment.
Also, thinking defensively, could you add a "choices" property to core/storage/user/gae_models.py to restrict the possible values, or some kind of validation in update_user_default_dashboard, or here? Basically, somewhere along the backend pipeline, we should make sure that only allowed values make it through the flow.
|
Also added backend tests. |
As decided in our meeting, we no longer need to make a banner as all users have a default dashboard. Users who signup using the create button will have the creator dashboard set as default.